Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add compression to object store #264

Merged
merged 6 commits into from
Feb 5, 2024
Merged

Conversation

Jarema
Copy link
Member

@Jarema Jarema commented Feb 5, 2024

This commit also cleans up formatting of the spec.

This commit also cleans up formatting of the spec.

Signed-off-by: Tomasz Pietrek <[email protected]>
@Jarema Jarema requested a review from ripienaar February 5, 2024 15:07
Signed-off-by: Tomasz Pietrek <[email protected]>
Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks some unintended spacing changes crept in

But mainly this doesnt seem to describe what the compression option does? see recent addition to KV adr for sample verbiage etc

adr/ADR-20.md Outdated

```go
type ObjectInfo struct {
ObjectMeta

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure these spaces are desired, also below and above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those spaces were removed in this PR, not added (by markdown linter). I have no idea why you would want them here, but the docs renders the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh god, need more coffee sorry, thought you were adding them lol.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right I see whats happening, you're removing spaces fine - but I think the entire line should be removed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right, removed them entirely.
It was just linter cleaning things up. I didn't want to change more than required for this PR, as I will cleanup the ADR as follow up :).

adr/ADR-20.md Outdated Show resolved Hide resolved
@Jarema
Copy link
Member Author

Jarema commented Feb 5, 2024

@ripienaar the verbiage was compied from KV ADR PR, but I realized it was improved as a follow up :).

Improved it a bit.
However in general, this ADR is not as detailed and fleshed out as the KV one. I will clean it up in separate PR to get it up to the standard.

@Jarema Jarema requested a review from ripienaar February 5, 2024 15:39
adr/ADR-20.md Outdated Show resolved Hide resolved
Signed-off-by: Tomasz Pietrek <[email protected]>
Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jarema Jarema merged commit 65b277d into main Feb 5, 2024
1 check passed
@Jarema Jarema mentioned this pull request Feb 5, 2024
16 tasks
@piotrpio piotrpio deleted the add-compression-to-object-store branch February 8, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants